Skip to content

agent: @U0AJM7X8FBR Chat - we want an onboarding flow that gets the user from #1575

Open
sweetmantech wants to merge 7 commits intotestfrom
agent/-u0ajm7x8fbr--chat---we-want-a-1773286899498
Open

agent: @U0AJM7X8FBR Chat - we want an onboarding flow that gets the user from #1575
sweetmantech wants to merge 7 commits intotestfrom
agent/-u0ajm7x8fbr--chat---we-want-a-1773286899498

Conversation

@sweetmantech
Copy link
Copy Markdown
Collaborator

@sweetmantech sweetmantech commented Mar 12, 2026

Automated PR from coding agent.

Summary by CodeRabbit

Release Notes

  • New Features
    • Introduced comprehensive onboarding wizard for new users with guided multi-step setup flow
    • Added artist discovery with live Spotify search and manual entry option
    • Enabled platform connection setup (Google Sheets, Drive, TikTok, Docs) during onboarding
    • Added Pulse feature configuration option during initial setup
    • Included personalized welcome experience and celebratory completion screen

@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
recoup-chat Ready Ready Preview Mar 12, 2026 2:28pm

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 12, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

A comprehensive multi-step onboarding wizard flow is introduced, guiding users through role selection, personal context, Spotify artist discovery, platform connections, Pulse setup, and auto-queued tasks before completing with a celebratory dashboard entry. Integration with HomePage mounts the modal on app initialization.

Changes

Cohort / File(s) Summary
Integration
components/Home/HomePage.tsx
Added OnboardingModal import and rendering preceding the Chat component.
Core Orchestration
components/Onboarding/OnboardingModal.tsx, components/Onboarding/useOnboarding.ts
OnboardingModal orchestrates a fixed 8-step progression; useOnboarding manages modal state, current step, data persistence, and navigation controls with auto-open logic triggered by incomplete onboarding status.
Welcome & Role Selection
components/Onboarding/OnboardingWelcomeStep.tsx, components/Onboarding/OnboardingRoleStep.tsx
OnboardingWelcomeStep auto-advances through animated research lines with personalized greeting; OnboardingRoleStep presents role grid with animated selection and Continue button.
User Context
components/Onboarding/OnboardingContextStep.tsx
Collects user name and company via pre-filled inputs, leveraging user provider data and role-based dynamic labels.
Artist Discovery
components/Onboarding/OnboardingArtistsStep.tsx, components/Onboarding/useSpotifyArtistSearch.ts
OnboardingArtistsStep integrates live Spotify search with debounced queries, avatar display, and manual fallback for artist selection; useSpotifyArtistSearch hook encapsulates API calls and loading state.
Connections & Features
components/Onboarding/OnboardingConnectionsStep.tsx, components/Onboarding/OnboardingPulseStep.tsx
OnboardingConnectionsStep triggers OAuth-like connector flows (Google Sheets, Drive, TikTok, Docs) with connection state tracking; OnboardingPulseStep toggles Pulse feature with benefits list and dynamic button labels.
Tasks & Completion
components/Onboarding/OnboardingTasksStep.tsx, components/Onboarding/OnboardingCompleteStep.tsx
OnboardingTasksStep displays role-tailored auto-queued tasks with progressive reveals; OnboardingCompleteStep renders celebratory UI with confetti animation, AI summary, and dashboard navigation.
Navigation & Configuration
components/Onboarding/OnboardingNavButtons.tsx, components/Onboarding/OnboardingStepDots.tsx, components/Onboarding/onboardingRoleConfig.ts
OnboardingNavButtons provides reusable back/next controls; OnboardingStepDots renders progress dots; onboardingRoleConfig centralizes role metadata (labels, icons, descriptions, field labels).
Persistence
components/Onboarding/useOnboardingPersist.ts
Hook handles post-completion side effects: parallel artist creation, Pulse activation, account metadata persistence via API, and sidebar refresh.

Sequence Diagram

sequenceDiagram
    participant User
    participant HomePage as HomePage
    participant Modal as OnboardingModal
    participant Hook as useOnboarding
    participant Steps as Step Components
    participant API as Backend/APIs
    
    User->>HomePage: Load app
    HomePage->>Modal: Render OnboardingModal
    Modal->>Hook: useOnboarding()
    Hook->>Hook: Check userData.onboarding_status
    Hook->>Modal: { isOpen: true, step: 'welcome' }
    Modal->>Steps: Render OnboardingWelcomeStep
    
    User->>Steps: Progress through steps (role → context → artists → connections → pulse → tasks)
    Steps->>Hook: onSelect/onUpdate/onNext
    Hook->>Hook: Update step & data
    Hook->>Modal: Re-render with new step
    
    loop Artist Search
        User->>Steps: Type artist query
        Steps->>API: fetch /api/spotify/search (debounced)
        API-->>Steps: SpotifyArtist[]
        Steps->>Steps: Display results dropdown
    end
    
    loop Connect Platforms
        User->>Steps: Click Connect button
        Steps->>API: authorizeConnectorApi()
        API-->>User: OAuth flow (new tab)
        User->>Steps: Connection established
        Steps->>Hook: onConnect(slug)
    end
    
    User->>Steps: Complete final step (tasks)
    Steps->>Hook: onNext()
    Hook->>Hook: step = 'complete'
    Modal->>Steps: Render OnboardingCompleteStep
    User->>Steps: Click "Open my dashboard"
    Steps->>Hook: complete()
    Hook->>API: useOnboardingPersist.persist(data)
    API->>API: Create artists, activate Pulse, update account
    API-->>Hook: Success
    Hook->>Modal: isOpen = false
    Modal->>HomePage: Unmount, show Chat
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

This pull request introduces substantial new functionality spanning 16 files with heterogeneous logic patterns across 1600+ lines. The complexity arises from: (1) diverse step logic ranging from API integrations (Spotify search, OAuth connector flows) to state management, (2) multi-component orchestration requiring careful prop threading and callback coordination, (3) integration with external systems (Spotify API, connector authorization), and (4) persistence logic handling parallel operations and error scenarios. While individual step components follow consistent patterns, each exhibits unique business logic requiring separate reasoning.

Poem

🎵 Welcome, dear user, to the onboarding flow,
Where roles align and artists steal the show,
From Spotify searches to pulse so bright,
Eight steps of magic leading to the light—
Your dashboard awaits! 🚀✨

🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Solid & Clean Code ⚠️ Warning OnboardingArtistsStep (200+ lines) violates SRP by managing search UI, artist caching, fallback logic, and state simultaneously. OnboardingCompleteStep handles animations, confetti, formatting, and navigation. useOnboardingPersist orchestrates artist creation, Pulse activation, and API calls in parallel. Extract hardcoded timing values to onboardingConstants.ts. Split OnboardingArtistsStep into SearchUI and ArtistManagement subcomponents. Consolidate persistence logic into a single sequenced flow with proper error handling and user feedback.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch agent/-u0ajm7x8fbr--chat---we-want-a-1773286899498
📝 Coding Plan for PR comments
  • Generate coding plan

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d88963caf7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +59 to +62
saveArtist({
name: a.name,
spotifyUrl: a.spotifyUrl,
accountId: userData.account_id,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Send a valid payload when creating onboarding artists

In the researching step, saveArtist is called with { name, spotifyUrl, accountId }, but /api/artist/profile only consumes fields like artistId, email, and profileUrls and the create path requires email (app/api/artist/profile/route.ts, lib/supabase/artist/updateArtistProfile.tsx). For new users this request returns an error instead of creating artists, so onboarding appears to succeed while no artist records are actually created.

Useful? React with 👍 / 👎.

Comment on lines +73 to +74
onboardingStatus: { completed: true, completedAt: new Date().toISOString() },
onboardingData: data,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Persist onboarding completion with fields the API updates

The completion call posts onboardingStatus/onboardingData, but /api/account/update does not read or write those keys (it only maps fields like roleType/companyName into account_info). This means onboarding_status is never persisted, so useOnboarding will treat the same user as not onboarded on later loads and reopen the modal.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
components/Onboarding/OnboardingContextStep.tsx (1)

23-57: Use a real form for this step.

These fields are collecting submit-able data, but the current div wrapper loses Enter-to-continue and form semantics. Wrapping them in a <form onSubmit={...}> and making the CTA a submit button will improve keyboard flow without adding complexity.

As per coding guidelines, "Use semantic HTML elements appropriate to the component's role" and "Ensure keyboard navigation and focus management in UI components".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingContextStep.tsx` around lines 23 - 57, The
inputs are inside plain divs so Enter-to-continue and form semantics are lost;
wrap the input block in a <form> element and wire its onSubmit to call the
existing onNext handler (use an inline handler that does e.preventDefault() then
calls onNext if needed) so keyboard submit works, then change the CTA Button to
be a submit button (remove the onClick). Keep the existing controlled inputs and
handlers (onChangeName, onChangeCompany) intact so only the wrapper and Button
type/prop change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/Onboarding/OnboardingArtistsStep.tsx`:
- Around line 72-78: The remove artist icon buttons lack accessible names;
update the button that calls removeArtist(i) to include an aria-label like
`Remove artist {artist.name}` (use the artist variable in scope) so screen
readers announce the action and target; likewise add an aria-label (e.g., "Add
artist" or "Add another artist") to the plus/add button referenced later (the
handler that adds an artist). Ensure these aria-labels are present on the button
elements that render the X icon and the plus icon so assistive tech announces
meaningful labels.

In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 102-115: The dialog lacks an accessible title—wire the visible
step heading into the dialog's accessible name by adding an ARIA title element
(or using your DialogTitle component) and linking it to the Dialog/DialogContent
so screen readers announce the current step; specifically, use STEP_TITLES[step]
(with a fallback like "Onboarding") as the title text, ensure the Dialog root
has role="dialog" and aria-modal="true" (or that your Dialog component sets
them), and include aria-labelledby (or aria-label) on the Dialog/DialogContent
referencing the title element so the researching/complete states still expose a
proper dialog name.
- Around line 67-76: The frontend is sending onboardingStatus and onboardingData
but the API handler in app/api/account/update/route.tsx never reads or persists
them, so userData.onboarding_status remains falsy and useOnboarding reopens the
wizard; update the server-side request handler to parse onboardingStatus and
onboardingData from the request body (the names sent by the fetch in
OnboardingModal.tsx) and write them into the account record (e.g., set
onboarding_status and onboarding_data fields) before returning the updated user,
or alternatively adjust the handler’s expected keys to match what
OnboardingModal.tsx sends so the onboarding completion write is actually
persisted.
- Around line 58-65: The current onboarding flow fires saveArtist() calls (see
artistPromises and await Promise.allSettled) but never checks results, so
onboarding can be marked complete even if all saves failed; modify either
saveArtist to throw on non-ok responses (e.g., throw when response.ok is false)
or inspect the Promise.allSettled results and only proceed to mark onboarding
complete when at least one result.status === "fulfilled" (or treat rejections as
failures). Also remove or implement handling for onboardingStatus /
onboardingData sent in the API call (they are currently ignored by the API),
either by deleting those fields from the request or updating the server-side
handler to extract and persist them. Finally add an accessible name to the modal
by setting aria-labelledby on DialogContent referencing the "Welcome to
Recoupable" header id (or add aria-label directly to DialogContent) so screen
readers can announce the dialog.

In `@components/Onboarding/OnboardingResearchingStep.tsx`:
- Around line 24-39: The useEffect is closing over a stale currentStage so
setCompletedStages always appends 0; fix by moving the completed-stages update
into the setCurrentStage functional updater so it uses the latest stage value
(use the prev argument inside setCurrentStage to compute next and update
setCompletedStages([...prevCompleted, prev]) accordingly), keep the
interval/clearInterval and onComplete behavior (call onComplete after reaching
RESEARCH_STAGES.length) and keep the effect deps minimal (onComplete only).

In `@components/Onboarding/OnboardingRoleStep.tsx`:
- Around line 33-50: Replace the current list of standalone buttons with an
accessible single-select radio group: wrap the choices in a fieldset/legend or
use `@radix-ui/react-radio-group`, render each role from ROLES as a RadioItem (or
input type="radio") instead of a button, and drive its checked state from
selected while invoking onSelect(role.id) on change; keep the visual classes
(cn, role.icon, role.label, role.description) but move them into the radio/item
content and ensure each item exposes aria-checked/checked and a unique key/id so
screen readers announce the selected role and keyboard navigation (arrow
keys/tab) follows native radio behavior.

---

Nitpick comments:
In `@components/Onboarding/OnboardingContextStep.tsx`:
- Around line 23-57: The inputs are inside plain divs so Enter-to-continue and
form semantics are lost; wrap the input block in a <form> element and wire its
onSubmit to call the existing onNext handler (use an inline handler that does
e.preventDefault() then calls onNext if needed) so keyboard submit works, then
change the CTA Button to be a submit button (remove the onClick). Keep the
existing controlled inputs and handlers (onChangeName, onChangeCompany) intact
so only the wrapper and Button type/prop change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d94d0f23-0d82-42ab-8ad0-438dddf582a3

📥 Commits

Reviewing files that changed from the base of the PR and between 0371429 and d88963c.

📒 Files selected for processing (8)
  • components/Home/HomePage.tsx
  • components/Onboarding/OnboardingArtistsStep.tsx
  • components/Onboarding/OnboardingCompleteStep.tsx
  • components/Onboarding/OnboardingContextStep.tsx
  • components/Onboarding/OnboardingModal.tsx
  • components/Onboarding/OnboardingResearchingStep.tsx
  • components/Onboarding/OnboardingRoleStep.tsx
  • components/Onboarding/useOnboarding.ts

Comment on lines +72 to +78
<button
type="button"
onClick={() => removeArtist(i)}
className="rounded-md p-1 hover:bg-muted transition-colors"
>
<X className="h-3.5 w-3.5 text-muted-foreground" />
</button>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Label the icon-only artist actions.

The remove buttons and the plus button have no accessible name, so screen readers will only announce button. Please add aria-labels here, including the artist name on remove.

As per coding guidelines, "Provide proper ARIA roles/states and test with screen readers" and "Provide clear labels and error messages in form components".

Also applies to: 94-102

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 72 - 78, The
remove artist icon buttons lack accessible names; update the button that calls
removeArtist(i) to include an aria-label like `Remove artist {artist.name}` (use
the artist variable in scope) so screen readers announce the action and target;
likewise add an aria-label (e.g., "Add artist" or "Add another artist") to the
plus/add button referenced later (the handler that adds an artist). Ensure these
aria-labels are present on the button elements that render the X icon and the
plus icon so assistive tech announces meaningful labels.

Comment on lines +58 to +65
const artistPromises = (data.artists ?? []).map(a =>
saveArtist({
name: a.name,
spotifyUrl: a.spotifyUrl,
accountId: userData.account_id,
}).catch(console.error),
);
await Promise.allSettled(artistPromises);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "OnboardingModal.tsx" | head -5

Repository: recoupable/chat

Length of output: 102


🏁 Script executed:

# Find the OnboardingModal file and examine its structure
find . -type f -name "*.tsx" | grep -i onboarding | head -10

Repository: recoupable/chat

Length of output: 354


🏁 Script executed:

# Check repository structure
git ls-files | grep -i "onboarding\|modal" | head -20

Repository: recoupable/chat

Length of output: 863


🏁 Script executed:

cat -n ./components/Onboarding/OnboardingModal.tsx | head -100

Repository: recoupable/chat

Length of output: 4181


🏁 Script executed:

# Get the full file to understand context
wc -l ./components/Onboarding/OnboardingModal.tsx

Repository: recoupable/chat

Length of output: 106


🏁 Script executed:

cat -n ./components/Onboarding/OnboardingModal.tsx | tail -100

Repository: recoupable/chat

Length of output: 3933


🏁 Script executed:

# Check the API route to see what fields it actually accepts
cat -n ./app/api/account/update/route.tsx | head -50

Repository: recoupable/chat

Length of output: 2158


🏁 Script executed:

# Check saveArtist function to understand what it does
cat -n ./lib/saveArtist.ts 2>/dev/null || cat -n ./lib/saveArtist.js 2>/dev/null || find . -name "saveArtist*" -type f

Repository: recoupable/chat

Length of output: 79


🏁 Script executed:

cat -n ./lib/saveArtist.tsx

Repository: recoupable/chat

Length of output: 545


🏁 Script executed:

# Check if the API route accepts onboardingStatus field - get more of the route
cat -n ./app/api/account/update/route.tsx | sed -n '1,60p'

Repository: recoupable/chat

Length of output: 2704


🏁 Script executed:

# Check the Dialog component to understand accessibility implementation
cat -n ./components/ui/dialog.tsx | head -80

Repository: recoupable/chat

Length of output: 3424


🏁 Script executed:

# Check if saveArtist throws on HTTP error or just returns error data
cat -n ./app/api/artist/profile/route.tsx 2>/dev/null | head -50 || find . -name "*artist/profile*" -type f

Repository: recoupable/chat

Length of output: 41


🏁 Script executed:

# Look for how errors are handled in the artists API
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -path "*/api/artist*" | head -10

Repository: recoupable/chat

Length of output: 262


🏁 Script executed:

# Verify the Promise behavior - check if there's error handling that matters
grep -r "onboardingStatus\|onboardingData" --include="*.ts" --include="*.tsx" | head -20

Repository: recoupable/chat

Length of output: 262


🏁 Script executed:

# Check the artist profile API route to see if it throws or returns errors
cat -n ./app/api/artist/profile/route.ts | head -60

Repository: recoupable/chat

Length of output: 2425


🏁 Script executed:

# Check if saveArtist throws on non-ok responses
cat -n ./lib/saveArtist.tsx

Repository: recoupable/chat

Length of output: 545


🏁 Script executed:

# Verify Promise.allSettled semantics - understand what happens with the caught promises
python3 << 'EOF'
# Demonstrate the Promise behavior described in the review
print("Promise behavior analysis:")
print("1. saveArtist() returns a fetch promise")
print("2. .catch(console.error) converts rejection → undefined (fulfilled)")
print("3. Promise.allSettled() waits for all to settle (fulfilled or rejected)")
print("")
print("Key issue: The .catch() handler turns a rejection into a fulfilled promise")
print("This means artistPromises array contains fulfilled promises (even if saveArtist failed)")
print("When Promise.allSettled() completes, it sees all 'fulfilled' results")
print("The code then proceeds to mark onboarding complete regardless of actual save success")
EOF

Repository: recoupable/chat

Length of output: 583


🏁 Script executed:

# Check if there's any validation that artists were actually created
grep -A 10 -B 5 "Promise.allSettled" ./components/Onboarding/OnboardingModal.tsx

Repository: recoupable/chat

Length of output: 630


🏁 Script executed:

# Check if saveArtist actually throws on HTTP errors
python3 << 'EOF'
print("Critical insight: saveArtist() behavior")
print("")
print("The function does NOT throw on HTTP 400:")
print("1. fetch() resolves even on 400 status")
print("2. response.json() parses both 200 and 400 responses")
print("3. saveArtist returns the parsed response object in both cases")
print("4. The promise RESOLVES (not rejects) with an error object")
print("")
print("Therefore:")
print("- .catch(console.error) does NOT trigger (no rejection)")
print("- The promise array contains RESOLVED promises with error objects")
print("- Promise.allSettled() sees them all as 'fulfilled'")
print("- Code proceeds without checking if saves actually succeeded")
print("")
print("The real problem: saveArtist() should check response.ok")
print("OR the code should inspect the allSettled results")
EOF

Repository: recoupable/chat

Length of output: 708


🏁 Script executed:

# Check if there's any validation after Promise.allSettled
cat -n ./components/Onboarding/OnboardingModal.tsx | sed -n '52,85p'

Repository: recoupable/chat

Length of output: 1448


Verify artist creation success before marking onboarding complete.

saveArtist() doesn't throw on HTTP errors—it resolves with an error object regardless of the response status. The code doesn't inspect the Promise.allSettled() results, so it proceeds to mark onboarding complete even when all artist creations failed. Either add a status check in saveArtist() (e.g., throw if !response.ok) or verify that the allSettled results contain successful outcomes before advancing.

Additionally, the API route ignores the onboardingStatus and onboardingData fields you're sending on lines 73-74—they're not extracted from the request body. Remove this dead code or implement support for persisting onboarding metadata.

Finally, the dialog lacks an explicit accessible name for screen readers. Add aria-labelledby to the DialogContent pointing to the "Welcome to Recoupable" header, or use aria-label directly on the content element.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 58 - 65, The current
onboarding flow fires saveArtist() calls (see artistPromises and await
Promise.allSettled) but never checks results, so onboarding can be marked
complete even if all saves failed; modify either saveArtist to throw on non-ok
responses (e.g., throw when response.ok is false) or inspect the
Promise.allSettled results and only proceed to mark onboarding complete when at
least one result.status === "fulfilled" (or treat rejections as failures). Also
remove or implement handling for onboardingStatus / onboardingData sent in the
API call (they are currently ignored by the API), either by deleting those
fields from the request or updating the server-side handler to extract and
persist them. Finally add an accessible name to the modal by setting
aria-labelledby on DialogContent referencing the "Welcome to Recoupable" header
id (or add aria-label directly to DialogContent) so screen readers can announce
the dialog.

Comment on lines +67 to +76
// Mark onboarding complete on account_info
await fetch("/api/account/update", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
accountId: userData.account_id,
onboardingStatus: { completed: true, completedAt: new Date().toISOString() },
onboardingData: data,
}),
}).catch(console.error);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

This completion write is currently a no-op.

app/api/account/update/route.tsx, Lines 8-40 never reads onboardingStatus or onboardingData. Those fields are dropped here, so userData.onboarding_status stays falsy and useOnboarding will reopen the wizard on the next load.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 67 - 76, The frontend
is sending onboardingStatus and onboardingData but the API handler in
app/api/account/update/route.tsx never reads or persists them, so
userData.onboarding_status remains falsy and useOnboarding reopens the wizard;
update the server-side request handler to parse onboardingStatus and
onboardingData from the request body (the names sent by the fetch in
OnboardingModal.tsx) and write them into the account record (e.g., set
onboarding_status and onboarding_data fields) before returning the updated user,
or alternatively adjust the handler’s expected keys to match what
OnboardingModal.tsx sends so the onboarding completion write is actually
persisted.

Comment on lines +102 to +115
<Dialog open={isOpen} onOpenChange={() => {}}>
<DialogContent
className="max-w-lg p-0 gap-0 overflow-hidden [&>button]:hidden"
onInteractOutside={e => e.preventDefault()}
onEscapeKeyDown={e => e.preventDefault()}
>
{/* Header bar */}
{STEP_TITLES[step] && (
<div className="flex items-center justify-between border-b px-6 py-4">
<div className="flex items-center gap-2">
<span className="text-sm font-medium">Welcome to Recoupable</span>
</div>
<span className="text-xs text-muted-foreground">{STEP_TITLES[step]}</span>
</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Give the modal an accessible title.

The visual header is not exposed as the dialog's accessible name, and the researching/complete states render no header at all. Please wire the current step heading into the dialog title so screen readers know what just opened.

As per coding guidelines, "Use role=\"dialog\" and aria-modal=\"true\" for modal/dialog components" and "Provide proper ARIA roles/states and test with screen readers".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 102 - 115, The dialog
lacks an accessible title—wire the visible step heading into the dialog's
accessible name by adding an ARIA title element (or using your DialogTitle
component) and linking it to the Dialog/DialogContent so screen readers announce
the current step; specifically, use STEP_TITLES[step] (with a fallback like
"Onboarding") as the title text, ensure the Dialog root has role="dialog" and
aria-modal="true" (or that your Dialog component sets them), and include
aria-labelledby (or aria-label) on the Dialog/DialogContent referencing the
title element so the researching/complete states still expose a proper dialog
name.

Comment on lines +24 to +39
useEffect(() => {
const interval = setInterval(() => {
setCompletedStages(prev => [...prev, currentStage]);
setCurrentStage(prev => {
const next = prev + 1;
if (next >= RESEARCH_STAGES.length) {
clearInterval(interval);
setTimeout(onComplete, 600);
return prev;
}
return next;
});
}, 900);

return () => clearInterval(interval);
}, [onComplete]); // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "OnboardingResearchingStep.tsx" | head -5

Repository: recoupable/chat

Length of output: 112


🏁 Script executed:

cat -n ./components/Onboarding/OnboardingResearchingStep.tsx

Repository: recoupable/chat

Length of output: 4102


The progress effect captures a stale currentStage value, breaking stage completion tracking.

The effect closure over currentStage = 0 means setCompletedStages always receives 0 in line 26, regardless of the actual current stage. This results in completedStages = [0, 0, 0, 0, 0, 0], so stages 1–5 never render as completed. The issue stems from omitting currentStage from the dependency array despite reading it directly.

Suggested fix

Move setCompletedStages into the setCurrentStage callback to read the correct stage value:

 useEffect(() => {
   const interval = setInterval(() => {
     setCurrentStage(prev => {
+      setCompletedStages(done => [...done, prev]);
       const next = prev + 1;
       if (next >= RESEARCH_STAGES.length) {
         clearInterval(interval);
         setTimeout(onComplete, 600);
         return prev;
       }
       return next;
     });
   }, 900);

   return () => clearInterval(interval);
-}, [onComplete]); // eslint-disable-line react-hooks/exhaustive-deps
+}, [onComplete]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingResearchingStep.tsx` around lines 24 - 39,
The useEffect is closing over a stale currentStage so setCompletedStages always
appends 0; fix by moving the completed-stages update into the setCurrentStage
functional updater so it uses the latest stage value (use the prev argument
inside setCurrentStage to compute next and update
setCompletedStages([...prevCompleted, prev]) accordingly), keep the
interval/clearInterval and onComplete behavior (call onComplete after reaching
RESEARCH_STAGES.length) and keep the effect deps minimal (onComplete only).

Comment on lines +33 to +50
<div className="grid grid-cols-2 gap-3 sm:grid-cols-3">
{ROLES.map(role => (
<button
key={role.id}
type="button"
onClick={() => onSelect(role.id)}
className={cn(
"flex flex-col items-start gap-1 rounded-xl border-2 p-4 text-left transition-all hover:border-primary hover:bg-primary/5",
selected === role.id
? "border-primary bg-primary/10"
: "border-border bg-muted/30",
)}
>
<span className="text-2xl">{role.icon}</span>
<span className="text-sm font-semibold">{role.label}</span>
<span className="text-xs text-muted-foreground">{role.description}</span>
</button>
))}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Expose the role picker as a single-select group.

This is visually one choice, but assistive tech sees six unrelated buttons with no announced selected state. Please switch to a semantic radio group/fieldset (or equivalent) so the current role is exposed and keyboard behavior is predictable.

As per coding guidelines, "Provide proper ARIA roles/states and test with screen readers", "Support fieldset/legend for grouped inputs in form components", and "Use @radix-ui/react-* primitives for behavior and accessibility".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingRoleStep.tsx` around lines 33 - 50, Replace
the current list of standalone buttons with an accessible single-select radio
group: wrap the choices in a fieldset/legend or use `@radix-ui/react-radio-group`,
render each role from ROLES as a RadioItem (or input type="radio") instead of a
button, and drive its checked state from selected while invoking
onSelect(role.id) on change; keep the visual classes (cn, role.icon, role.label,
role.description) but move them into the radio/item content and ensure each item
exposes aria-checked/checked and a unique key/id so screen readers announce the
selected role and keyboard navigation (arrow keys/tab) follows native radio
behavior.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (3)
components/Onboarding/OnboardingModal.tsx (3)

119-125: ⚠️ Potential issue | 🟠 Major

Add accessible dialog title for screen readers.

The modal lacks an accessible name. Screen reader users won't know what dialog just opened. Per coding guidelines, dialogs require role="dialog" and proper labeling.

♻️ Suggested fix with DialogTitle
+import { Dialog, DialogContent, DialogTitle } from "@/components/ui/dialog";
+
+const STEP_TITLES: Record<Step, string> = {
+  welcome: "Welcome",
+  role: "Select your role",
+  context: "Tell us about yourself",
+  artists: "Add your artists",
+  connections: "Connect your tools",
+  pulse: "Turn on Pulse",
+  tasks: "Your tasks",
+  complete: "You're all set",
+};
+
 <DialogContent
   className="max-w-lg p-0 gap-0 overflow-hidden [&>button]:hidden"
   onInteractOutside={e => e.preventDefault()}
   onEscapeKeyDown={e => e.preventDefault()}
 >
+  <DialogTitle className="sr-only">{STEP_TITLES[step as Step]}</DialogTitle>
   {/* Header */}

As per coding guidelines: "Use role=\"dialog\" and aria-modal=\"true\" for modal/dialog components" and "Provide proper ARIA roles/states."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 119 - 125, The Dialog
lacks an accessible name/role for screen readers; update the OnboardingModal to
provide role="dialog" and aria-modal="true" on the dialog container (the Dialog
or DialogContent element) and add a visible or visually-hidden title element
referenced by aria-labelledby (use an existing DialogTitle component or add one
and set its id, then set aria-labelledby to that id) so screen readers announce
the dialog name; ensure onOpenChange handler remains and remove the empty inline
noop if necessary.

61-68: ⚠️ Potential issue | 🟠 Major

Artist creation failures are silently ignored.

saveArtist() doesn't throw on HTTP errors — it returns the error response as a resolved promise. The .catch(console.error) only catches network failures, and Promise.allSettled() sees all promises as "fulfilled" regardless of actual success.

Consider checking the response status either in saveArtist or after allSettled:

♻️ Suggested validation
 const artistPromises = (data.artists ?? []).map(a =>
   saveArtist({
     name: a.name,
     spotifyUrl: a.spotifyUrl,
     accountId: userData.account_id,
-  }).catch(console.error),
+  }),
 );
-await Promise.allSettled(artistPromises);
+const results = await Promise.allSettled(artistPromises);
+const failures = results.filter(
+  r => r.status === "rejected" || (r.status === "fulfilled" && r.value?.error)
+);
+if (failures.length > 0) {
+  console.error("Some artists failed to save:", failures);
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 61 - 68, The artist
creation logic currently swallows server-side failures because saveArtist
returns error responses as resolved promises and the current
.catch(console.error) only handles network errors; update the flow so failures
are detected and handled: either modify saveArtist to throw on non-2xx responses
(so callers can catch) or, after creating artistPromises in OnboardingModal (the
artistPromises array and the await Promise.allSettled(...) call), inspect the
results from Promise.allSettled for any result.status === "fulfilled" whose
value indicates an error response or result.status === "rejected" and
surface/log/propagate those failures (e.g., collect failed artists and show a
user-facing error or retry), ensuring you reference saveArtist and the
artistPromises/Promise.allSettled block when making the change.

75-96: ⚠️ Potential issue | 🔴 Critical

Onboarding completion status is not persisted by the API.

Lines 84-94 send onboardingStatus and onboardingData to /api/account/update, but per the API route at app/api/account/update/route.tsx (lines 7-40), these fields are never extracted from the request body or written to the database.

This means userData.onboarding_status remains null, and useOnboarding will reopen the wizard on the next page load — users will be stuck in an infinite onboarding loop.

Fix required in the API route to destructure and persist these fields:

// In app/api/account/update/route.tsx
const { ..., onboardingStatus, onboardingData } = body;

// Then in updateAccountInfo call:
await updateAccountInfo(accountId, {
  ...
  onboarding_status: onboardingStatus,
  onboarding_data: onboardingData,
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 75 - 96, The API
route handler currently ignores onboardingStatus and onboardingData sent from
the client; update the request body destructuring to include onboardingStatus
and onboardingData (e.g., const { accountId, ..., onboardingStatus,
onboardingData } = body) and pass them into the database update call that uses
updateAccountInfo so the persisted object includes onboarding_status:
onboardingStatus and onboarding_data: onboardingData (ensure you still map other
fields like accountId and existing payloads when calling updateAccountInfo).
🧹 Nitpick comments (5)
components/Onboarding/OnboardingModal.tsx (1)

107-117: Navigation timeout isn't cleaned up on unmount.

The setTimeout on line 115 isn't cleaned up if the component unmounts before it fires. While unlikely in this flow, it's a good practice to clean up all timers:

♻️ Suggested cleanup with ref
+const navigationTimeoutRef = useRef<ReturnType<typeof setTimeout>>();
+
+useEffect(() => {
+  return () => {
+    if (navigationTimeoutRef.current) clearTimeout(navigationTimeoutRef.current);
+  };
+}, []);
+
 const handleComplete = useCallback(async () => {
   await complete();
   const artistNames = (data.artists ?? []).map(a => a.name);
   if (artistNames.length > 0) {
     const q = encodeURIComponent(
       `Give me a complete status report for ${artistNames[0]} — ...`,
     );
-    setTimeout(() => { window.location.href = `/?q=${q}`; }, 200);
+    navigationTimeoutRef.current = setTimeout(() => { window.location.href = `/?q=${q}`; }, 200);
   }
 }, [complete, data.artists]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 107 - 117, The
setTimeout started in handleComplete isn't cleared on unmount; store the timer
id in a ref (e.g., timeoutRef) when you call setTimeout in handleComplete (and
clear any existing timeout before assigning), and add a useEffect cleanup that
calls clearTimeout(timeoutRef.current) and nulls it on unmount to prevent the
delayed navigation from firing after the component is unmounted; reference
handleComplete, complete, data.artists and the setTimeout call.
components/Onboarding/OnboardingPulseStep.tsx (1)

38-56: Add keyboard accessibility to the toggle card.

The clickable div acts as a toggle control but lacks keyboard support. Users relying on keyboards or assistive technologies won't be able to interact with it effectively.

Consider adding role="button", tabIndex={0}, and an onKeyDown handler for Enter/Space:

♻️ Suggested improvement
       <div
         className={cn(
           "rounded-xl border-2 p-5 transition-all cursor-pointer",
           enabled
             ? "border-primary bg-primary/8"
             : "border-border bg-muted/20",
         )}
+        role="button"
+        tabIndex={0}
         onClick={() => onToggle(!enabled)}
+        onKeyDown={e => {
+          if (e.key === "Enter" || e.key === " ") {
+            e.preventDefault();
+            onToggle(!enabled);
+          }
+        }}
+        aria-pressed={enabled}
       >

As per coding guidelines: "Ensure keyboard navigation and focus management in UI components" and "Provide proper ARIA roles/states."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingPulseStep.tsx` around lines 38 - 56, The
clickable card wrapping the Switch lacks keyboard/ARIA support; add
role="button", tabIndex={0}, aria-pressed={enabled} and an onKeyDown handler on
the same div that listens for Enter/Space and calls onToggle(!enabled)
(mirroring the onClick behavior and preventing default/propagation as needed),
and ensure the Switch props (checked/onCheckedChange) remain intact so screen
readers get the correct state; use the existing onToggle and enabled symbols to
implement this.
components/Onboarding/OnboardingConnectionsStep.tsx (1)

99-100: Add accessible text for the connected state indicator.

Screen reader users won't know a connector is connected — the CheckCircle2 icon is purely visual. Add visually hidden text:

♻️ Suggested fix
 {isConnected ? (
-  <CheckCircle2 className="h-5 w-5 text-primary shrink-0" />
+  <>
+    <CheckCircle2 className="h-5 w-5 text-primary shrink-0" aria-hidden="true" />
+    <span className="sr-only">Connected</span>
+  </>
 ) : (

As per coding guidelines: "Provide proper ARIA roles/states and test with screen readers."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingConnectionsStep.tsx` around lines 99 - 100,
Add accessible text for the connected state by rendering a visually hidden label
when isConnected is true: keep the CheckCircle2 icon for sighted users but mark
it aria-hidden="true", and add a span (e.g., className="sr-only" or your
project's equivalent) containing an explicit status string like "Connected" next
to the icon inside the OnboardingConnectionsStep component so screen readers
announce the connected state; ensure the text is only present when isConnected
is true and avoid duplicate announcements.
components/Onboarding/useOnboarding.ts (1)

55-61: Consider defensive handling for unexpected onboarding_status shapes.

The type assertion on line 57 assumes onboarding_status is either null or an object with a completed property. If the database contains an unexpected shape (e.g., a string or array), status?.completed could behave unexpectedly.

A more defensive check:

♻️ Suggested improvement
 useEffect(() => {
   if (!userData) return;
-  const status = userData.onboarding_status as Record<string, unknown> | null;
-  if (!status?.completed) {
+  const status = userData.onboarding_status;
+  const isCompleted =
+    typeof status === "object" &&
+    status !== null &&
+    "completed" in status &&
+    status.completed === true;
+  if (!isCompleted) {
     setIsOpen(true);
   }
 }, [userData]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useOnboarding.ts` around lines 55 - 61, The
onboarding_status property is being asserted to an object and read without
validation; update the useEffect in useOnboarding (the block that reads
userData.onboarding_status and calls setIsOpen) to defensively validate the
shape before checking completed: first guard that userData?.onboarding_status is
an object (typeof === 'object' && not null && !Array.isArray), then narrow it to
a Record<string, unknown> and only read status.completed if that validation
passes; treat non-object shapes (string, array, boolean) as "not completed" or
handle per business rule so you avoid reading undefined properties and
unexpected behavior.
components/Onboarding/OnboardingTasksStep.tsx (1)

64-72: Progressive reveal works but uses mutable closure variable.

The pattern of mutating i inside the interval closure works, but it's a bit unusual. An alternative using functional state update would be more idiomatic:

♻️ Optional refactor
 useEffect(() => {
-  let i = 0;
   const interval = setInterval(() => {
-    setRevealed(prev => [...prev, i]);
-    i++;
-    if (i >= tasks.length) clearInterval(interval);
+    setRevealed(prev => {
+      if (prev.length >= tasks.length) {
+        clearInterval(interval);
+        return prev;
+      }
+      return [...prev, prev.length];
+    });
   }, 180);
   return () => clearInterval(interval);
 }, [tasks.length]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingTasksStep.tsx` around lines 64 - 72, Replace
the mutable closure counter `i` with a functional state-driven update: inside
the effect keep the interval but call `setRevealed(prev => { ... })` to compute
the next index from `prev.length` (so you append the next task index based on
current `revealed` length) and use that same updater to detect when you've
reached `tasks.length` and then call `clearInterval(interval)`; reference the
existing symbols `setRevealed`, `revealed` (via its length), `tasks.length`, and
`clearInterval(interval)` to locate where to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/Onboarding/OnboardingConnectionsStep.tsx`:
- Around line 50-67: The handleConnect function currently calls onConnect(slug)
immediately after authorizeConnectorApi returns a URL, which marks the connector
as connected before OAuth completes; update this by either (A) removing the
onConnect call from handleConnect and instead invoking onConnect only from the
OAuth callback handler that confirms successful authorization, or (B) if
immediate signaling is intended, rename onConnect to onConnectInitiated (and
update its usages and docs) to reflect initiation rather than completion; also
replace the empty catch in handleConnect with proper error handling that
surfaces failures (e.g., call an onError/onConnectError callback or set an error
state) and log the caught error so authorizeConnectorApi failures are visible.
Ensure changes reference handleConnect, onConnect/onConnectInitiated,
authorizeConnectorApi, setConnecting and any OAuth callback handler you add or
modify.

In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 56-105: The effect running on step change can execute its async
run() multiple times during re-renders because dependencies like data, userData,
accessToken, and getArtists are omitted; add a persistent guard ref (e.g.,
hasRunRef via useRef(false)) checked at the top of the useEffect and set to true
before starting the async work so the body (the run() that calls saveArtist,
updatePulse, fetch("/api/account/update"), and getArtists) only executes once
per transition to step === "tasks"; reset or clear the ref when step changes
away if you need to allow reruns on future transitions.

In `@components/Onboarding/OnboardingWelcomeStep.tsx`:
- Around line 31-45: The effect sets a delayed setTimeout(onDone, 600) inside
the interval but never clears that timeout on unmount; create a timeoutRef
(useRef) to store the timeout ID and assign the ID when you call setTimeout in
the block where next >= RESEARCH_LINES.length, then
clearTimeout(timeoutRef.current) in the effect cleanup (and when you clear the
interval) to prevent onDone from firing after unmount; update references in this
component (useEffect, setLineIdx callback, onDone, setDone) to use the
timeoutRef for safe cleanup.

---

Duplicate comments:
In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 119-125: The Dialog lacks an accessible name/role for screen
readers; update the OnboardingModal to provide role="dialog" and
aria-modal="true" on the dialog container (the Dialog or DialogContent element)
and add a visible or visually-hidden title element referenced by aria-labelledby
(use an existing DialogTitle component or add one and set its id, then set
aria-labelledby to that id) so screen readers announce the dialog name; ensure
onOpenChange handler remains and remove the empty inline noop if necessary.
- Around line 61-68: The artist creation logic currently swallows server-side
failures because saveArtist returns error responses as resolved promises and the
current .catch(console.error) only handles network errors; update the flow so
failures are detected and handled: either modify saveArtist to throw on non-2xx
responses (so callers can catch) or, after creating artistPromises in
OnboardingModal (the artistPromises array and the await Promise.allSettled(...)
call), inspect the results from Promise.allSettled for any result.status ===
"fulfilled" whose value indicates an error response or result.status ===
"rejected" and surface/log/propagate those failures (e.g., collect failed
artists and show a user-facing error or retry), ensuring you reference
saveArtist and the artistPromises/Promise.allSettled block when making the
change.
- Around line 75-96: The API route handler currently ignores onboardingStatus
and onboardingData sent from the client; update the request body destructuring
to include onboardingStatus and onboardingData (e.g., const { accountId, ...,
onboardingStatus, onboardingData } = body) and pass them into the database
update call that uses updateAccountInfo so the persisted object includes
onboarding_status: onboardingStatus and onboarding_data: onboardingData (ensure
you still map other fields like accountId and existing payloads when calling
updateAccountInfo).

---

Nitpick comments:
In `@components/Onboarding/OnboardingConnectionsStep.tsx`:
- Around line 99-100: Add accessible text for the connected state by rendering a
visually hidden label when isConnected is true: keep the CheckCircle2 icon for
sighted users but mark it aria-hidden="true", and add a span (e.g.,
className="sr-only" or your project's equivalent) containing an explicit status
string like "Connected" next to the icon inside the OnboardingConnectionsStep
component so screen readers announce the connected state; ensure the text is
only present when isConnected is true and avoid duplicate announcements.

In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 107-117: The setTimeout started in handleComplete isn't cleared on
unmount; store the timer id in a ref (e.g., timeoutRef) when you call setTimeout
in handleComplete (and clear any existing timeout before assigning), and add a
useEffect cleanup that calls clearTimeout(timeoutRef.current) and nulls it on
unmount to prevent the delayed navigation from firing after the component is
unmounted; reference handleComplete, complete, data.artists and the setTimeout
call.

In `@components/Onboarding/OnboardingPulseStep.tsx`:
- Around line 38-56: The clickable card wrapping the Switch lacks keyboard/ARIA
support; add role="button", tabIndex={0}, aria-pressed={enabled} and an
onKeyDown handler on the same div that listens for Enter/Space and calls
onToggle(!enabled) (mirroring the onClick behavior and preventing
default/propagation as needed), and ensure the Switch props
(checked/onCheckedChange) remain intact so screen readers get the correct state;
use the existing onToggle and enabled symbols to implement this.

In `@components/Onboarding/OnboardingTasksStep.tsx`:
- Around line 64-72: Replace the mutable closure counter `i` with a functional
state-driven update: inside the effect keep the interval but call
`setRevealed(prev => { ... })` to compute the next index from `prev.length` (so
you append the next task index based on current `revealed` length) and use that
same updater to detect when you've reached `tasks.length` and then call
`clearInterval(interval)`; reference the existing symbols `setRevealed`,
`revealed` (via its length), `tasks.length`, and `clearInterval(interval)` to
locate where to implement this change.

In `@components/Onboarding/useOnboarding.ts`:
- Around line 55-61: The onboarding_status property is being asserted to an
object and read without validation; update the useEffect in useOnboarding (the
block that reads userData.onboarding_status and calls setIsOpen) to defensively
validate the shape before checking completed: first guard that
userData?.onboarding_status is an object (typeof === 'object' && not null &&
!Array.isArray), then narrow it to a Record<string, unknown> and only read
status.completed if that validation passes; treat non-object shapes (string,
array, boolean) as "not completed" or handle per business rule so you avoid
reading undefined properties and unexpected behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c31f1ae6-c180-47a8-acc2-271349f357cc

📥 Commits

Reviewing files that changed from the base of the PR and between d88963c and a11b4c7.

📒 Files selected for processing (7)
  • components/Onboarding/OnboardingCompleteStep.tsx
  • components/Onboarding/OnboardingConnectionsStep.tsx
  • components/Onboarding/OnboardingModal.tsx
  • components/Onboarding/OnboardingPulseStep.tsx
  • components/Onboarding/OnboardingTasksStep.tsx
  • components/Onboarding/OnboardingWelcomeStep.tsx
  • components/Onboarding/useOnboarding.ts

Comment on lines +50 to +67
const handleConnect = async (slug: string) => {
if (!accessToken) return;
setConnecting(slug);
try {
const url = await authorizeConnectorApi(accessToken, {
connector: slug,
callbackUrl: window.location.href,
});
if (url) {
onConnect(slug);
window.open(url, "_blank", "noopener,noreferrer");
}
} catch {
// silently continue
} finally {
setConnecting(null);
}
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

onConnect is called before OAuth actually completes.

The onConnect(slug) call on line 59 fires immediately when the OAuth window opens, not when the user successfully authorizes. This marks the connection as "connected" in the UI even if the user closes the OAuth window without completing authorization.

Consider either:

  1. Moving onConnect to a callback URL handler that confirms success, or
  2. Renaming to onConnectInitiated to reflect the actual behavior and documenting that the UI shows "pending" connections.

Additionally, the empty catch block silently swallows errors — users get no feedback when authorization fails.

♻️ Suggested improvement with user feedback
+import { toast } from "sonner";
+
 const handleConnect = async (slug: string) => {
   if (!accessToken) return;
   setConnecting(slug);
   try {
     const url = await authorizeConnectorApi(accessToken, {
       connector: slug,
       callbackUrl: window.location.href,
     });
     if (url) {
-      onConnect(slug);
       window.open(url, "_blank", "noopener,noreferrer");
+      // Note: onConnect should ideally be called after OAuth callback confirms success
+      onConnect(slug);
     }
-  } catch {
-    // silently continue
+  } catch (err) {
+    toast.error(`Failed to connect ${slug}. Please try again.`);
   } finally {
     setConnecting(null);
   }
 };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingConnectionsStep.tsx` around lines 50 - 67,
The handleConnect function currently calls onConnect(slug) immediately after
authorizeConnectorApi returns a URL, which marks the connector as connected
before OAuth completes; update this by either (A) removing the onConnect call
from handleConnect and instead invoking onConnect only from the OAuth callback
handler that confirms successful authorization, or (B) if immediate signaling is
intended, rename onConnect to onConnectInitiated (and update its usages and
docs) to reflect initiation rather than completion; also replace the empty catch
in handleConnect with proper error handling that surfaces failures (e.g., call
an onError/onConnectError callback or set an error state) and log the caught
error so authorizeConnectorApi failures are visible. Ensure changes reference
handleConnect, onConnect/onConnectInitiated, authorizeConnectorApi,
setConnecting and any OAuth callback handler you add or modify.

Comment on lines +31 to +45
useEffect(() => {
const interval = setInterval(() => {
setLineIdx(prev => {
const next = prev + 1;
if (next >= RESEARCH_LINES.length) {
clearInterval(interval);
setDone(true);
setTimeout(onDone, 600);
return prev;
}
return next;
});
}, 500);
return () => clearInterval(interval);
}, [onDone]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clean up the delayed onDone timeout on unmount.

The setTimeout(onDone, 600) on line 38 isn't cleaned up if the component unmounts before it fires. This could trigger navigation or state updates on an unmounted component.

♻️ Suggested fix using a ref
+import { useEffect, useState, useRef } from "react";
+
 export function OnboardingWelcomeStep({ onDone }: Props) {
   const { email, userData } = useUserProvider();
   const [lineIdx, setLineIdx] = useState(0);
   const [done, setDone] = useState(false);
+  const timeoutRef = useRef<ReturnType<typeof setTimeout>>();

   const displayName =
     userData?.name || (email ? email.split("@")[0] : null);

   useEffect(() => {
     const interval = setInterval(() => {
       setLineIdx(prev => {
         const next = prev + 1;
         if (next >= RESEARCH_LINES.length) {
           clearInterval(interval);
           setDone(true);
-          setTimeout(onDone, 600);
+          timeoutRef.current = setTimeout(onDone, 600);
           return prev;
         }
         return next;
       });
     }, 500);
-    return () => clearInterval(interval);
+    return () => {
+      clearInterval(interval);
+      if (timeoutRef.current) clearTimeout(timeoutRef.current);
+    };
   }, [onDone]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingWelcomeStep.tsx` around lines 31 - 45, The
effect sets a delayed setTimeout(onDone, 600) inside the interval but never
clears that timeout on unmount; create a timeoutRef (useRef) to store the
timeout ID and assign the ID when you call setTimeout in the block where next >=
RESEARCH_LINES.length, then clearTimeout(timeoutRef.current) in the effect
cleanup (and when you clear the interval) to prevent onDone from firing after
unmount; update references in this component (useEffect, setLineIdx callback,
onDone, setDone) to use the timeoutRef for safe cleanup.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
components/Onboarding/OnboardingContextStep.tsx (1)

43-55: Consider a guard ref instead of disabling exhaustive-deps.

The effect intentionally runs once to pre-fill the name, but disabling the lint rule hides the implicit contract. A cleaner pattern uses a hasPrefilledRef guard:

♻️ Suggested improvement
+const hasPrefilledRef = useRef(false);
+
 useEffect(() => {
-  if (!name && (userData?.name || email)) {
+  if (hasPrefilledRef.current) return;
+  if (!name && (userData?.name || email)) {
+    hasPrefilledRef.current = true;
     const inferred = /* ... */;
     if (inferred) onChangeName(inferred);
   }
-}, [userData?.name, email]); // eslint-disable-line react-hooks/exhaustive-deps
+}, [name, userData?.name, email, onChangeName]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingContextStep.tsx` around lines 43 - 55,
Replace the disabled exhaustive-deps pattern in the useEffect that pre-fills the
name by adding a persistent guard ref (e.g., hasPrefilledRef) to ensure the
prefill runs only once; update the effect (the useEffect that reads
userData?.name and email and calls onChangeName) to check and set
hasPrefilledRef.current so you can safely include all real dependencies
(userData?.name, email, name, onChangeName) without disabling the lint rule.
components/Onboarding/OnboardingPulseStep.tsx (1)

39-57: Make the toggle card keyboard-accessible.

The card is clickable via mouse but not reachable by keyboard. Since the embedded Switch handles the actual toggle, users can still operate it, but the card's click-to-toggle affordance is mouse-only. Consider adding tabIndex={0}, role="button", and an onKeyDown handler for Enter/Space:

♻️ Suggested improvement
 <div
   className={cn(
     "rounded-xl border-2 p-5 transition-all cursor-pointer",
     enabled ? "border-primary bg-primary/8" : "border-border bg-muted/20",
   )}
   onClick={() => onToggle(!enabled)}
+  onKeyDown={e => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); onToggle(!enabled); } }}
+  tabIndex={0}
+  role="button"
+  aria-pressed={enabled}
 >
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingPulseStep.tsx` around lines 39 - 57, The card
div in OnboardingPulseStep is only mouse-clickable; make it keyboard-accessible
by adding tabIndex={0}, role="button", and an onKeyDown handler that listens for
Enter and Space and calls onToggle(!enabled) (preventDefault for Space) to
mirror the onClick behavior; ensure the onKeyDown does not interfere with the
inner Switch (keep the Switch's onClick stopPropagation) and update the
accessible attributes so visual focus and screen readers can interact with the
card.
components/Onboarding/OnboardingStepDots.tsx (1)

3-11: Import the canonical OnboardingStep type instead of redefining it.

This local type duplicates OnboardingStep from useOnboarding.ts. If the step list evolves, this copy will drift. Import the shared type to maintain a single source of truth:

♻️ Suggested refactor
+import { type OnboardingStep } from "./useOnboarding";
 import { cn } from "@/lib/utils";

-type OnboardingStep =
-  | "welcome"
-  | "role"
-  | "context"
-  | "artists"
-  | "connections"
-  | "pulse"
-  | "tasks"
-  | "complete";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingStepDots.tsx` around lines 3 - 11, Remove the
duplicated local OnboardingStep union and instead import the canonical
OnboardingStep from the shared onboarding module (useOnboarding.ts); delete the
local type declaration in OnboardingStepDots.tsx, add an import for
OnboardingStep at the top, and ensure any references in the file (e.g., props,
state, or function signatures that used the local type) use the imported
OnboardingStep type so there’s a single source of truth.
components/Onboarding/OnboardingTasksStep.tsx (1)

65-73: Consider using state for the reveal counter for clearer intent.

The current approach with a mutable i variable works, but mixing mutable closure variables with React state updates can be tricky to reason about. A cleaner pattern uses an interval that increments a counter state:

♻️ Optional refactor
-const [revealed, setRevealed] = useState<number[]>([]);
+const [revealedCount, setRevealedCount] = useState(0);

 useEffect(() => {
-  let i = 0;
   const interval = setInterval(() => {
-    setRevealed(prev => [...prev, i]);
-    i++;
-    if (i >= tasks.length) clearInterval(interval);
+    setRevealedCount(prev => {
+      if (prev >= tasks.length) {
+        clearInterval(interval);
+        return prev;
+      }
+      return prev + 1;
+    });
   }, 180);
   return () => clearInterval(interval);
 }, [tasks.length]);

-// In render: revealed.includes(i)
+// In render: i < revealedCount
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingTasksStep.tsx` around lines 65 - 73, Replace
the mutable closure counter with React state: create a reveal counter state
(e.g., revealIndex via useState) and update it inside the useEffect interval
instead of using the local variable i; in the same effect increment revealIndex
with setRevealIndex(prev => prev + 1) on each tick, push the new index into
setRevealed (or derive revealed from revealIndex) and clear the interval when
revealIndex >= tasks.length or on cleanup. Update references in the component to
use revealIndex and ensure the effect depends on tasks.length and revealIndex
(or only tasks.length if deriving revealed inside the effect) so the interval
logic in useEffect, setRevealed, and the cleanup are deterministic and avoid
closure-stale values.
components/Onboarding/useOnboarding.ts (1)

55-61: Consider stricter type validation for onboarding_status.

The database schema shows onboarding_status is typed as Json | null, which can be any JSON value—strings, numbers, booleans, arrays, or objects. The current type assertion to Record<string, unknown> doesn't guard against non-object values.

If onboarding_status were set to a string, number, or array, the check !status?.completed would still evaluate to true (since those types don't have a completed property), potentially opening the modal unintentionally.

♻️ Proposed type guard for safer validation
 useEffect(() => {
   if (!userData) return;
-  const status = userData.onboarding_status as Record<string, unknown> | null;
-  if (!status?.completed) {
+  const status = userData.onboarding_status;
+  const isCompleted =
+    typeof status === "object" &&
+    status !== null &&
+    !Array.isArray(status) &&
+    "completed" in status &&
+    status.completed === true;
+  if (!isCompleted) {
     setIsOpen(true);
   }
 }, [userData]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useOnboarding.ts` around lines 55 - 61, The current
cast of userData.onboarding_status to Record<string, unknown> is unsafe; update
the useEffect to perform a runtime type guard on onboarding_status (check it is
non-null, typeof === 'object', not an array) and then verify the completed
property exists and is a boolean (e.g., 'completed' in status && typeof
status.completed === 'boolean') before deciding to call setIsOpen(true); locate
the useEffect that reads userData and replace the direct cast with this guard so
non-object JSON (string/number/array) won't incorrectly open the modal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/Onboarding/OnboardingArtistsStep.tsx`:
- Around line 11-15: Artist images are being dropped because ArtistEntry has
imageUrl but OnboardingArtist does not; update the onboarding types and save
flow to preserve it: add imageUrl?: string to the OnboardingArtist type in
useOnboarding.ts, update the saveArtist signature/handler there to accept and
store imageUrl, and pass imageUrl from the place that calls saveArtist in
OnboardingModal.tsx (where name and spotifyUrl are currently passed). Ensure any
state/reducer or persistence logic that consumes OnboardingArtist is updated to
handle the new imageUrl field.

In `@components/Onboarding/useOnboarding.ts`:
- Around line 82-87: The complete() returned by useOnboarding only closes the
modal and doesn't persist onboarding state; modify useOnboarding to accept an
optional onComplete callback (or an optional flag to persist) and have complete
invoke that callback (or call the persistence function to update
userData.onboarding_status via your existing update API) before calling
setIsOpen(false); update the return signature of useOnboarding and document that
consuming components (e.g., OnboardingModal) must provide the onComplete handler
if they want persistent completion.
- Line 4: The import in useOnboarding.ts references a misspelled provider
filename; rename the provider file from UserProvder.tsx to UserProvider.tsx and
update the import in components/Onboarding/useOnboarding.ts to import {
useUserProvider } from "@/providers/UserProvider"; also search for other usages
of UserProvder (e.g., any imports of useUserProvider) and update them to the
corrected filename to ensure builds and imports resolve.

---

Nitpick comments:
In `@components/Onboarding/OnboardingContextStep.tsx`:
- Around line 43-55: Replace the disabled exhaustive-deps pattern in the
useEffect that pre-fills the name by adding a persistent guard ref (e.g.,
hasPrefilledRef) to ensure the prefill runs only once; update the effect (the
useEffect that reads userData?.name and email and calls onChangeName) to check
and set hasPrefilledRef.current so you can safely include all real dependencies
(userData?.name, email, name, onChangeName) without disabling the lint rule.

In `@components/Onboarding/OnboardingPulseStep.tsx`:
- Around line 39-57: The card div in OnboardingPulseStep is only
mouse-clickable; make it keyboard-accessible by adding tabIndex={0},
role="button", and an onKeyDown handler that listens for Enter and Space and
calls onToggle(!enabled) (preventDefault for Space) to mirror the onClick
behavior; ensure the onKeyDown does not interfere with the inner Switch (keep
the Switch's onClick stopPropagation) and update the accessible attributes so
visual focus and screen readers can interact with the card.

In `@components/Onboarding/OnboardingStepDots.tsx`:
- Around line 3-11: Remove the duplicated local OnboardingStep union and instead
import the canonical OnboardingStep from the shared onboarding module
(useOnboarding.ts); delete the local type declaration in OnboardingStepDots.tsx,
add an import for OnboardingStep at the top, and ensure any references in the
file (e.g., props, state, or function signatures that used the local type) use
the imported OnboardingStep type so there’s a single source of truth.

In `@components/Onboarding/OnboardingTasksStep.tsx`:
- Around line 65-73: Replace the mutable closure counter with React state:
create a reveal counter state (e.g., revealIndex via useState) and update it
inside the useEffect interval instead of using the local variable i; in the same
effect increment revealIndex with setRevealIndex(prev => prev + 1) on each tick,
push the new index into setRevealed (or derive revealed from revealIndex) and
clear the interval when revealIndex >= tasks.length or on cleanup. Update
references in the component to use revealIndex and ensure the effect depends on
tasks.length and revealIndex (or only tasks.length if deriving revealed inside
the effect) so the interval logic in useEffect, setRevealed, and the cleanup are
deterministic and avoid closure-stale values.

In `@components/Onboarding/useOnboarding.ts`:
- Around line 55-61: The current cast of userData.onboarding_status to
Record<string, unknown> is unsafe; update the useEffect to perform a runtime
type guard on onboarding_status (check it is non-null, typeof === 'object', not
an array) and then verify the completed property exists and is a boolean (e.g.,
'completed' in status && typeof status.completed === 'boolean') before deciding
to call setIsOpen(true); locate the useEffect that reads userData and replace
the direct cast with this guard so non-object JSON (string/number/array) won't
incorrectly open the modal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9d2d4d19-4362-40bd-9b08-27b576e3bf29

📥 Commits

Reviewing files that changed from the base of the PR and between a11b4c7 and 50e81ca.

📒 Files selected for processing (11)
  • components/Onboarding/OnboardingArtistsStep.tsx
  • components/Onboarding/OnboardingCompleteStep.tsx
  • components/Onboarding/OnboardingConnectionsStep.tsx
  • components/Onboarding/OnboardingContextStep.tsx
  • components/Onboarding/OnboardingModal.tsx
  • components/Onboarding/OnboardingPulseStep.tsx
  • components/Onboarding/OnboardingRoleStep.tsx
  • components/Onboarding/OnboardingStepDots.tsx
  • components/Onboarding/OnboardingTasksStep.tsx
  • components/Onboarding/OnboardingWelcomeStep.tsx
  • components/Onboarding/useOnboarding.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • components/Onboarding/OnboardingConnectionsStep.tsx
  • components/Onboarding/OnboardingWelcomeStep.tsx

Comment on lines +11 to +15
interface ArtistEntry {
name: string;
spotifyUrl?: string;
imageUrl?: string;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify if imageUrl is used elsewhere or only for local display
rg -n "imageUrl" --type=ts --type=tsx -C2

Repository: recoupable/chat

Length of output: 86


🏁 Script executed:

#!/bin/bash
# Search for imageUrl usage with corrected syntax
rg -n "imageUrl" -C2

Repository: recoupable/chat

Length of output: 16532


🏁 Script executed:

#!/bin/bash
# Find and examine the OnboardingArtistsStep.tsx file
fd "OnboardingArtistsStep.tsx" -type f

Repository: recoupable/chat

Length of output: 229


🏁 Script executed:

#!/bin/bash
# Find and examine the useOnboarding.ts file
fd "useOnboarding" -type f

Repository: recoupable/chat

Length of output: 229


🏁 Script executed:

#!/bin/bash
# Find useOnboarding hook
find . -name "*useOnboarding*" -type f

Repository: recoupable/chat

Length of output: 99


🏁 Script executed:

#!/bin/bash
# Read the OnboardingArtistsStep.tsx file to see the full context
head -250 components/Onboarding/OnboardingArtistsStep.tsx

Repository: recoupable/chat

Length of output: 9199


🏁 Script executed:

#!/bin/bash
# Search for where onUpdate is called and how data flows
rg -n "onUpdate" components/Onboarding/OnboardingArtistsStep.tsx -B2 -A2

Repository: recoupable/chat

Length of output: 1091


🏁 Script executed:

#!/bin/bash
# Read the useOnboarding.ts hook to check OnboardingArtist type and saveArtist function
cat components/Onboarding/useOnboarding.ts

Repository: recoupable/chat

Length of output: 2349


🏁 Script executed:

#!/bin/bash
# Check where OnboardingArtistsStep is used and how the onUpdate callback flows
rg -n "OnboardingArtistsStep" -A5 -B2

Repository: recoupable/chat

Length of output: 2409


🏁 Script executed:

#!/bin/bash
# Search for saveArtist function or where data is persisted
rg -n "saveArtist"

Repository: recoupable/chat

Length of output: 460


🏁 Script executed:

#!/bin/bash
# Check how the onboarding component uses updateData from useOnboarding
rg -n "updateData.*artists" -C3

Repository: recoupable/chat

Length of output: 656


🏁 Script executed:

#!/bin/bash
# Read the OnboardingModal.tsx to see the complete flow and how artists data is handled
sed -n '1,100p' components/Onboarding/OnboardingModal.tsx

Repository: recoupable/chat

Length of output: 3578


🏁 Script executed:

#!/bin/bash
# Check the complete method in OnboardingModal to see what happens when onboarding completes
rg -n "complete|saveArtist" components/Onboarding/OnboardingModal.tsx -B3 -A5

Repository: recoupable/chat

Length of output: 3572


Align ArtistEntry and OnboardingArtist types for imageUrl

The ArtistEntry interface captures imageUrl from Spotify (line 85) and uses it locally for UI display, but the canonical OnboardingArtist type in useOnboarding.ts excludes it entirely. When artists are persisted via saveArtist in OnboardingModal.tsx (lines 56-61), only name and spotifyUrl are passed—imageUrl is discarded.

Either:

  1. Add imageUrl to OnboardingArtist and propagate it through saveArtist, or
  2. Remove imageUrl from ArtistEntry if it's only needed for transient UI display during onboarding
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 11 - 15, Artist
images are being dropped because ArtistEntry has imageUrl but OnboardingArtist
does not; update the onboarding types and save flow to preserve it: add
imageUrl?: string to the OnboardingArtist type in useOnboarding.ts, update the
saveArtist signature/handler there to accept and store imageUrl, and pass
imageUrl from the place that calls saveArtist in OnboardingModal.tsx (where name
and spotifyUrl are currently passed). Ensure any state/reducer or persistence
logic that consumes OnboardingArtist is updated to handle the new imageUrl
field.

"use client";

import { useCallback, useEffect, useState } from "react";
import { useUserProvider } from "@/providers/UserProvder";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the actual filename in the providers directory
fd -t f -e ts -e tsx "UserProv" providers/

Repository: recoupable/chat

Length of output: 84


Rename UserProvder.tsx to UserProvider.tsx — the filename has a typo.

The import path correctly references the existing file, but providers/UserProvder.tsx is misspelled. Rename it to UserProvider.tsx and update this import accordingly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useOnboarding.ts` at line 4, The import in
useOnboarding.ts references a misspelled provider filename; rename the provider
file from UserProvder.tsx to UserProvider.tsx and update the import in
components/Onboarding/useOnboarding.ts to import { useUserProvider } from
"@/providers/UserProvider"; also search for other usages of UserProvder (e.g.,
any imports of useUserProvider) and update them to the corrected filename to
ensure builds and imports resolve.

Comment on lines +82 to +87
const complete = useCallback(() => {
setIsOpen(false);
}, []);

return { isOpen, step, data, updateData, nextStep, prevStep, complete };
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

complete() doesn't persist completion status — modal may reopen on remount.

The complete function only closes the modal by setting isOpen to false. Since the useEffect checks userData.onboarding_status on mount, if that value isn't updated before the component remounts (or page refreshes), the onboarding modal will reopen.

Given the docstring states this "fires once for new accounts," consider whether persistence should be coordinated here or if the consuming component (OnboardingModal) is responsible. If the latter, document this contract clearly.

💡 Option A: Accept callback to signal completion
-export function useOnboarding() {
+export function useOnboarding(onComplete?: () => Promise<void> | void) {
   // ...existing code...

-  const complete = useCallback(() => {
+  const complete = useCallback(async () => {
+    await onComplete?.();
     setIsOpen(false);
   }, []);
💡 Option B: Document the persistence contract
+/**
+ * Closes the onboarding modal.
+ * 
+ * NOTE: Callers must persist `onboarding_status.completed = true` to the
+ * database before or after calling this function to prevent the modal
+ * from reopening on subsequent mounts.
+ */
 const complete = useCallback(() => {
   setIsOpen(false);
 }, []);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useOnboarding.ts` around lines 82 - 87, The complete()
returned by useOnboarding only closes the modal and doesn't persist onboarding
state; modify useOnboarding to accept an optional onComplete callback (or an
optional flag to persist) and have complete invoke that callback (or call the
persistence function to update userData.onboarding_status via your existing
update API) before calling setIsOpen(false); update the return signature of
useOnboarding and document that consuming components (e.g., OnboardingModal)
must provide the onComplete handler if they want persistent completion.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
components/Onboarding/OnboardingNavButtons.tsx (1)

21-24: Set explicit button types on these reusable actions.

Native buttons default to submit, so this nav row can trigger accidental form submissions if a later step wraps its content in a <form>. Setting type="button" here keeps the component safe to reuse.

Suggested change
-      <Button variant="outline" onClick={onBack} className="w-24">
+      <Button type="button" variant="outline" onClick={onBack} className="w-24">
         ← Back
       </Button>
-      <Button onClick={onNext} disabled={nextDisabled} className="flex-1">
+      <Button type="button" onClick={onNext} disabled={nextDisabled} className="flex-1">
         {nextLabel}
       </Button>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingNavButtons.tsx` around lines 21 - 24, The
Back and Next buttons (Button components using onBack and onNext with prop
nextDisabled) lack explicit type attributes and will default to type="submit",
risking accidental form submissions; update the Button instances for the Back
(onBack) and Next (onNext) actions to include type="button" so they behave as
non-submitting nav controls while preserving existing props like variant,
disabled and className.
components/Onboarding/onboardingRoleConfig.ts (1)

70-80: Reuse the canonical "other" config in the fallback.

This duplicates data that already exists in ONBOARDING_ROLES, so the two definitions can drift. Returning ROLE_CONFIG_MAP.other keeps the fallback aligned with the source of truth.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/onboardingRoleConfig.ts` around lines 70 - 80, The
fallback in getRoleConfig currently returns a hard-coded "other" object that
duplicates ONBOARDING_ROLES; change it to return the canonical entry from
ROLE_CONFIG_MAP instead (i.e., use ROLE_CONFIG_MAP["other"] or
ROLE_CONFIG_MAP.other) so the fallback stays in sync with the source of truth;
update getRoleConfig to return ROLE_CONFIG_MAP[roleId ?? ""] ??
ROLE_CONFIG_MAP.other and remove the duplicated literal object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/Onboarding/OnboardingArtistsStep.tsx`:
- Around line 71-97: The search input (Input) should be converted to an
accessible combobox: add a programmatic label or aria-label, set role="combobox"
with aria-expanded tied to showDropdown, aria-controls pointing to the results
container id, and aria-activedescendant referencing the active option id; add
component state activeIndex (default -1) and pass highlighted state/id to each
ArtistSearchResult so each option has role="option" and an id; enhance the
onKeyDown handler to handle ArrowDown/ArrowUp/Home/End to move activeIndex (and
open dropdown), Enter to select the highlighted option via
addFromSpotify(results[activeIndex]) (or call addManual when no selection),
Escape to close (setFocused(false)/showDropdown false), and manage focus/blur
timing so clicking an option doesn't immediately close the list (use onMouseDown
on options or preventDefault on blur), ensuring keyboard-only users can navigate
and select results and screen readers get correct ARIA roles and state.
- Around line 35-53: Both addFromSpotify and addManual perform duplicate checks
with different rules; extract a single predicate (e.g., isDuplicateArtist) and
use it from both functions so the same logic decides duplicates before mutating
artists. Implement isDuplicateArtist to accept a candidate object (with optional
spotifyUrl and name) and return true if any existing artist matches by
spotifyUrl when present, otherwise by case-insensitive name; call this predicate
at the start of addFromSpotify and addManual and return early if true, then
proceed to call onUpdate and clearResults.

In `@components/Onboarding/useOnboardingPersist.ts`:
- Around line 28-33: The artist payload sent to saveArtist in
useOnboardingPersist.ts is using {name, spotifyUrl, accountId} but the
downstream /api/artist/profile expects the API contract’s identifier/profile URL
fields; update the mapping for the artistPromises (where (data.artists ??
[]).map(...) calls saveArtist) to transform each OnboardingArtist into the API
shape (e.g., map spotifyUrl -> profileUrl or identifier field used by the API,
include any required id/identifier fields and accountId) before calling
saveArtist so the body matches the profile API contract and avoids creating
incomplete records.
- Around line 43-63: The POST to "/api/account/update" in persist
(useOnboardingPersist.ts) is sending onboardingStatus/onboardingData but the
server route drops them and the fetch's .catch won't catch HTTP 4xx/5xx; change
the request to either call a dedicated onboarding endpoint (e.g.,
"/api/onboarding/save" or extend the server handler for account.update to accept
onboardingStatus/onboardingData) and update the request body accordingly, then
await the fetch response and check response.ok (throw or return a rejected
Promise on non-2xx) so the hook fails on server errors and the UI can surface
the error; update the function around the existing fetch call to implement this
error handling and endpoint change (reference: useOnboardingPersist.persist or
the fetch call shown).

In `@components/Onboarding/useSpotifyArtistSearch.ts`:
- Around line 26-49: The search useCallback in useSpotifyArtistSearch.ts can
apply stale fetch results (older queries) and overwrite newer state; update it
to ignore or cancel stale responses by attaching an AbortController to the fetch
or using a monotonic requestId stored in a ref (e.g., searchRequestIdRef)
incremented for each call and captured by each invocation, then only call
setResults/setSearching if the response’s requestId matches the latest; also
ensure you abort on new searches and on unmount or clear pending state so the
finally block doesn’t flip searching for a stale request (apply same fix to the
other search-like block around lines 52-58).

---

Nitpick comments:
In `@components/Onboarding/OnboardingNavButtons.tsx`:
- Around line 21-24: The Back and Next buttons (Button components using onBack
and onNext with prop nextDisabled) lack explicit type attributes and will
default to type="submit", risking accidental form submissions; update the Button
instances for the Back (onBack) and Next (onNext) actions to include
type="button" so they behave as non-submitting nav controls while preserving
existing props like variant, disabled and className.

In `@components/Onboarding/onboardingRoleConfig.ts`:
- Around line 70-80: The fallback in getRoleConfig currently returns a
hard-coded "other" object that duplicates ONBOARDING_ROLES; change it to return
the canonical entry from ROLE_CONFIG_MAP instead (i.e., use
ROLE_CONFIG_MAP["other"] or ROLE_CONFIG_MAP.other) so the fallback stays in sync
with the source of truth; update getRoleConfig to return ROLE_CONFIG_MAP[roleId
?? ""] ?? ROLE_CONFIG_MAP.other and remove the duplicated literal object.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8ee026fd-ef07-45ec-9df1-163930c74683

📥 Commits

Reviewing files that changed from the base of the PR and between 50e81ca and 1df4f3d.

📒 Files selected for processing (11)
  • components/Onboarding/OnboardingArtistsStep.tsx
  • components/Onboarding/OnboardingConnectionsStep.tsx
  • components/Onboarding/OnboardingContextStep.tsx
  • components/Onboarding/OnboardingModal.tsx
  • components/Onboarding/OnboardingNavButtons.tsx
  • components/Onboarding/OnboardingPulseStep.tsx
  • components/Onboarding/OnboardingRoleStep.tsx
  • components/Onboarding/OnboardingTasksStep.tsx
  • components/Onboarding/onboardingRoleConfig.ts
  • components/Onboarding/useOnboardingPersist.ts
  • components/Onboarding/useSpotifyArtistSearch.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • components/Onboarding/OnboardingModal.tsx
  • components/Onboarding/OnboardingTasksStep.tsx
  • components/Onboarding/OnboardingPulseStep.tsx
  • components/Onboarding/OnboardingRoleStep.tsx
  • components/Onboarding/OnboardingContextStep.tsx
  • components/Onboarding/OnboardingConnectionsStep.tsx

Comment on lines +35 to +53
const addFromSpotify = (a: SpotifyArtist) => {
if (artists.some(x => x.spotifyUrl === a.external_urls.spotify)) return;
onUpdate([
...artists,
{
name: a.name,
spotifyUrl: a.external_urls.spotify,
imageUrl: a.images?.[0]?.url,
},
]);
clearResults();
};

const addManual = () => {
const trimmed = query.trim();
if (!trimmed || artists.some(x => x.name.toLowerCase() === trimmed.toLowerCase())) return;
onUpdate([...artists, { name: trimmed }]);
clearResults();
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unify duplicate detection across manual and Spotify adds.

Right now the two add paths use different rules, so a manually added artist can still be added again from Spotify. This should share one duplicate check before either path mutates the list.

Suggested change
+  const normalizeArtistName = (value: string) => value.trim().toLowerCase();
+  const hasArtist = (name: string, spotifyUrl?: string) =>
+    artists.some(existing =>
+      (spotifyUrl && existing.spotifyUrl === spotifyUrl) ||
+      normalizeArtistName(existing.name) === normalizeArtistName(name),
+    );
+
   const addFromSpotify = (a: SpotifyArtist) => {
-    if (artists.some(x => x.spotifyUrl === a.external_urls.spotify)) return;
+    if (hasArtist(a.name, a.external_urls.spotify)) return;
     onUpdate([
       ...artists,
       {
         name: a.name,
         spotifyUrl: a.external_urls.spotify,
@@
   const addManual = () => {
     const trimmed = query.trim();
-    if (!trimmed || artists.some(x => x.name.toLowerCase() === trimmed.toLowerCase())) return;
+    if (!trimmed || hasArtist(trimmed)) return;
     onUpdate([...artists, { name: trimmed }]);
     clearResults();
   };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 35 - 53, Both
addFromSpotify and addManual perform duplicate checks with different rules;
extract a single predicate (e.g., isDuplicateArtist) and use it from both
functions so the same logic decides duplicates before mutating artists.
Implement isDuplicateArtist to accept a candidate object (with optional
spotifyUrl and name) and return true if any existing artist matches by
spotifyUrl when present, otherwise by case-insensitive name; call this predicate
at the start of addFromSpotify and addManual and return early if true, then
proceed to call onUpdate and clearResults.

Comment on lines +71 to +97
<div className="relative">
<div className="relative flex items-center">
<Search className="absolute left-3 h-4 w-4 text-muted-foreground pointer-events-none" />
<Input
placeholder={artistPlaceholder}
value={query}
onChange={e => setQuery(e.target.value)}
onFocus={() => setFocused(true)}
onBlur={() => setTimeout(() => setFocused(false), 150)}
onKeyDown={e => {
if (e.key === "Enter" && results.length > 0) addFromSpotify(results[0]);
else if (e.key === "Enter" && query.trim()) addManual();
}}
className="pl-9 pr-9"
/>
{searching && (
<Loader2 className="absolute right-3 h-4 w-4 animate-spin text-muted-foreground" />
)}
</div>

{/* Dropdown */}
{showDropdown && (
<div className="absolute z-50 mt-1 w-full rounded-xl border bg-popover shadow-lg overflow-hidden">
{results.map(a => (
<ArtistSearchResult key={a.id} artist={a} onSelect={addFromSpotify} />
))}
</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make this search control a real accessible combobox/listbox.

The field currently has no programmatic label, and keyboard users can only submit the first result with Enter. There’s no owned list state (aria-expanded/aria-controls), active option tracking, or arrow-key navigation through results, so this interaction is effectively mouse-first.

As per coding guidelines, "Ensure keyboard navigation and focus management in UI components" and "Provide proper ARIA roles/states and test with screen readers".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 71 - 97, The
search input (Input) should be converted to an accessible combobox: add a
programmatic label or aria-label, set role="combobox" with aria-expanded tied to
showDropdown, aria-controls pointing to the results container id, and
aria-activedescendant referencing the active option id; add component state
activeIndex (default -1) and pass highlighted state/id to each
ArtistSearchResult so each option has role="option" and an id; enhance the
onKeyDown handler to handle ArrowDown/ArrowUp/Home/End to move activeIndex (and
open dropdown), Enter to select the highlighted option via
addFromSpotify(results[activeIndex]) (or call addManual when no selection),
Escape to close (setFocused(false)/showDropdown false), and manage focus/blur
timing so clicking an option doesn't immediately close the list (use onMouseDown
on options or preventDefault on blur), ensuring keyboard-only users can navigate
and select results and screen readers get correct ARIA roles and state.

Comment on lines +28 to +33
const artistPromises = (data.artists ?? []).map(a =>
saveArtist({
name: a.name,
spotifyUrl: a.spotifyUrl,
accountId: userData.account_id,
}).catch(console.error),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

The artist payload here does not match the profile API contract.

This code saves { name, spotifyUrl, accountId }, but saveArtist forwards that body directly to /api/artist/profile, and the provided context shows the downstream API expects identifier/profile URL fields instead. With the current OnboardingArtist shape, this step can only fail or create incomplete artist records. Please normalize the onboarding selection into the API’s expected shape before calling saveArtist.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useOnboardingPersist.ts` around lines 28 - 33, The
artist payload sent to saveArtist in useOnboardingPersist.ts is using {name,
spotifyUrl, accountId} but the downstream /api/artist/profile expects the API
contract’s identifier/profile URL fields; update the mapping for the
artistPromises (where (data.artists ?? []).map(...) calls saveArtist) to
transform each OnboardingArtist into the API shape (e.g., map spotifyUrl ->
profileUrl or identifier field used by the API, include any required
id/identifier fields and accountId) before calling saveArtist so the body
matches the profile API contract and avoids creating incomplete records.

Comment on lines +43 to +63
await fetch("/api/account/update", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
accountId: userData.account_id,
roleType: data.roleType,
name: data.name,
companyName: data.companyName,
onboardingStatus: {
completed: true,
completedAt: new Date().toISOString(),
steps: {
role: data.roleType,
artistCount: (data.artists ?? []).length,
connectedCount: (data.connectedSlugs ?? []).length,
pulseEnabled: data.pulseEnabled,
},
},
onboardingData: data,
}),
}).catch(console.error);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This request cannot persist onboarding completion as written.

/api/account/update currently ignores onboardingStatus and onboardingData, so those values are dropped server-side. On top of that, fetch(...).catch(...) will not catch a 400/500, so persist still resolves as if onboarding saved successfully. Please either extend that route contract or send this to a dedicated onboarding endpoint, and fail the hook on non-2xx responses so the UI can surface the error.

Suggested local guard
-      await fetch("/api/account/update", {
+      const response = await fetch("/api/account/update", {
         method: "POST",
         headers: { "Content-Type": "application/json" },
         body: JSON.stringify({
           accountId: userData.account_id,
           roleType: data.roleType,
           name: data.name,
           companyName: data.companyName,
           onboardingStatus: {
             completed: true,
             completedAt: new Date().toISOString(),
             steps: {
               role: data.roleType,
               artistCount: (data.artists ?? []).length,
               connectedCount: (data.connectedSlugs ?? []).length,
               pulseEnabled: data.pulseEnabled,
             },
           },
           onboardingData: data,
         }),
-      }).catch(console.error);
+      });
+      if (!response.ok) {
+        throw new Error("Failed to persist onboarding");
+      }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useOnboardingPersist.ts` around lines 43 - 63, The POST
to "/api/account/update" in persist (useOnboardingPersist.ts) is sending
onboardingStatus/onboardingData but the server route drops them and the fetch's
.catch won't catch HTTP 4xx/5xx; change the request to either call a dedicated
onboarding endpoint (e.g., "/api/onboarding/save" or extend the server handler
for account.update to accept onboardingStatus/onboardingData) and update the
request body accordingly, then await the fetch response and check response.ok
(throw or return a rejected Promise on non-2xx) so the hook fails on server
errors and the UI can surface the error; update the function around the existing
fetch call to implement this error handling and endpoint change (reference:
useOnboardingPersist.persist or the fetch call shown).

Comment on lines +26 to +49
const search = useCallback(
async (q: string) => {
if (!q.trim() || q.length < 2) {
setResults([]);
return;
}
setSearching(true);
try {
const params = new URLSearchParams({ q, type: "artist", limit: "5" });
const url = `${NEW_API_BASE_URL}/api/spotify/search?${params}`;
const headers: HeadersInit = accessToken
? { Authorization: `Bearer ${accessToken}` }
: {};
const res = await fetch(url, { headers });
if (!res.ok) throw new Error("search failed");
const data = await res.json();
setResults(data?.artists?.items ?? []);
} catch {
setResults([]);
} finally {
setSearching(false);
}
},
[accessToken],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Ignore stale search responses before updating state.

This can show the wrong artists: if an older fetch resolves after a newer query, it still calls setResults/setSearching and overwrites the latest state. Clear/short queries have the same race.

Suggested change
   const [query, setQuery] = useState("");
   const [results, setResults] = useState<SpotifyArtist[]>([]);
   const [searching, setSearching] = useState(false);
   const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+  const requestIdRef = useRef(0);

   const search = useCallback(
     async (q: string) => {
+      const requestId = ++requestIdRef.current;
       if (!q.trim() || q.length < 2) {
         setResults([]);
+        setSearching(false);
         return;
       }
       setSearching(true);
       try {
         const params = new URLSearchParams({ q, type: "artist", limit: "5" });
         const url = `${NEW_API_BASE_URL}/api/spotify/search?${params}`;
         const headers: HeadersInit = accessToken
           ? { Authorization: `Bearer ${accessToken}` }
           : {};
         const res = await fetch(url, { headers });
         if (!res.ok) throw new Error("search failed");
         const data = await res.json();
+        if (requestId !== requestIdRef.current) return;
         setResults(data?.artists?.items ?? []);
       } catch {
+        if (requestId !== requestIdRef.current) return;
         setResults([]);
       } finally {
+        if (requestId !== requestIdRef.current) return;
         setSearching(false);
       }
     },
     [accessToken],
   );

Also applies to: 52-58

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/useSpotifyArtistSearch.ts` around lines 26 - 49, The
search useCallback in useSpotifyArtistSearch.ts can apply stale fetch results
(older queries) and overwrite newer state; update it to ignore or cancel stale
responses by attaching an AbortController to the fetch or using a monotonic
requestId stored in a ref (e.g., searchRequestIdRef) incremented for each call
and captured by each invocation, then only call setResults/setSearching if the
response’s requestId matches the latest; also ensure you abort on new searches
and on unmount or clear pending state so the finally block doesn’t flip
searching for a stale request (apply same fix to the other search-like block
around lines 52-58).

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (4)
components/Onboarding/OnboardingArtistsStep.tsx (3)

128-134: ⚠️ Potential issue | 🟠 Major

Give the remove button an accessible name.

The icon-only control still exposes just button to assistive tech. Add an aria-label that includes the artist name.

As per coding guidelines, "Provide proper ARIA roles/states and test with screen readers".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 128 - 134, The
remove button is icon-only and lacks an accessible name; update the button in
OnboardingArtistsStep (the button that calls remove(i) and renders <X />) to
include an aria-label that includes the artist's name (e.g., "Remove
{artist.name}") so screen readers get a meaningful label; ensure you use the
current item variable (the artist object from the array/map for index i) to
build the string and keep the visual markup unchanged.

33-49: ⚠️ Potential issue | 🟡 Minor

Use one duplicate check for both add paths.

addFromSpotify() only de-dupes by Spotify URL while addManual() only de-dupes by normalized name, so the same artist can still be added twice and later saved twice. Reuse a single predicate before either path mutates artists.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 33 - 49, The
duplicate-check logic is inconsistent: addFromSpotify (function addFromSpotify)
only checks spotifyUrl while addManual (function addManual) only checks
normalized name, allowing duplicates; refactor by creating a single predicate
(e.g., isDuplicate) that takes either a SpotifyArtist or a name and returns true
if artists contains an entry with the same normalized name or matching
spotifyUrl, then call this predicate from both addFromSpotify and addManual
before calling onUpdate/clearResults to prevent any duplicate insertions into
artists.

72-111: ⚠️ Potential issue | 🟠 Major

This search is still mouse-first.

The input/results pair still has no programmatic label, aria-expanded/aria-controls/aria-activedescendant, or arrow-key traversal, and both option buttons use onMouseDown, so Enter/Space on a focused option will not select it. Please wire this up as a real combobox/listbox interaction.

As per coding guidelines, "Ensure keyboard navigation and focus management in UI components" and "Provide proper ARIA roles/states and test with screen readers".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingArtistsStep.tsx` around lines 72 - 111, The
combobox is currently mouse-first; in OnboardingArtistsStep add proper ARIA and
keyboard handling by giving the Input an id and role="combobox" plus
aria-expanded and aria-controls pointing to the dropdown id, track an
activeIndex state and set aria-activedescendant to the focused option id when
showDropdown/showManualAdd is true, implement onKeyDown on the Input to handle
ArrowDown/ArrowUp to move activeIndex and Enter/Space to call
addFromSpotify(results[activeIndex]) or addManual, change the option elements in
the dropdown (rendered by ArtistSearchResult and the manual button) to have
role="option", unique ids, highlight based on activeIndex, and use onClick (and
also handle onKeyDown for Enter/Space) instead of onMouseDown so keyboard
selection and screen readers work correctly while keeping existing
addFromSpotify/addManual handlers.
components/Onboarding/OnboardingModal.tsx (1)

48-53: ⚠️ Potential issue | 🟠 Major

Give the dialog an accessible title.

DialogContent still opens without an accessible name, so screen readers will not announce what modal just appeared. Add a DialogTitle/aria-labelledby that tracks the current step, including the welcome and complete states.

As per coding guidelines, "Provide proper ARIA roles/states and test with screen readers".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 48 - 53, The modal
currently opens without an accessible name; add a DialogTitle (or set
aria-labelledby on DialogContent) that updates to reflect the current onboarding
step (including the welcome and complete states) so screen readers announce the
modal; update the component that renders DialogContent (the DialogContent
element) to include a DialogTitle element (or an id referenced by
aria-labelledby) that is derived from the step state/variable that drives the
content (e.g., the same state used for welcome/complete) and ensure the title
text changes with the step.
🧹 Nitpick comments (1)
components/Onboarding/OnboardingTasksStep.tsx (1)

7-8: Tie roleType to the shared onboarding role ids.

roleType: string | undefined plus Record<string, ...> means a typo or rename in components/Onboarding/onboardingRoleConfig.ts:15-64 quietly falls through to the default tasks on Line 62. Derive the role-id union from the shared config so this drift is caught at compile time.

Also applies to: 50-55, 61-62

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingTasksStep.tsx` around lines 7 - 8, Change the
loose string types to a compile-time linked role-id union: replace RoleTaskMap's
key type (currently Record<string, ...>) with a union derived from the shared
onboarding config (e.g. keyof typeof onboardingRoleConfig or an exported
OnboardingRoleId type) and update the roleType variable to that same union
(roleType: OnboardingRoleId | undefined). Import or derive the union from
components/Onboarding/onboardingRoleConfig (use keyof typeof
onboardingRoleConfig if no explicit type is exported) so any rename/typo in the
shared config fails to compile instead of falling back to defaults; update all
related usages (RoleTaskMap, the roleType declaration, and the lookup that falls
back to default tasks) to use the new union type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 28-32: The effect calling persist(data) in OnboardingModal's
useEffect is fire-and-forget and can be canceled by navigation; update the
finish flow to await the persist operation (or use navigator.sendBeacon / fetch
with keepalive) before proceeding to the hard navigation so the onboarding flag
is reliably written. Specifically, change the logic around persist(...) in the
effect and the code paths that trigger the hard navigation so they either await
the Promise returned by persist (from
components/Onboarding/useOnboardingPersist.ts's persist function) or switch the
POST to useBeacon/keepalive in useOnboardingPersist so the write survives
navigation; ensure any callers (the "tasks" step handler) are updated to depend
on the settled result.
- Around line 17-18: The code uses an undefined Step type and redundant casts
for the step variable; update OnboardingModal to remove the nonexistent Step
assertion (e.g., remove "as Step" at line 62) and delete unnecessary "as
OnboardingStep" casts (lines ~45 and ~59) so that the hook-returned step uses
the existing OnboardingStep type directly; verify PROGRESS_STEPS remains typed
as OnboardingStep[] and adjust any local variables or function signatures (e.g.,
the step parameter/constant and any usages in render/handlers) to rely on
OnboardingStep without additional casts.

---

Duplicate comments:
In `@components/Onboarding/OnboardingArtistsStep.tsx`:
- Around line 128-134: The remove button is icon-only and lacks an accessible
name; update the button in OnboardingArtistsStep (the button that calls
remove(i) and renders <X />) to include an aria-label that includes the artist's
name (e.g., "Remove {artist.name}") so screen readers get a meaningful label;
ensure you use the current item variable (the artist object from the array/map
for index i) to build the string and keep the visual markup unchanged.
- Around line 33-49: The duplicate-check logic is inconsistent: addFromSpotify
(function addFromSpotify) only checks spotifyUrl while addManual (function
addManual) only checks normalized name, allowing duplicates; refactor by
creating a single predicate (e.g., isDuplicate) that takes either a
SpotifyArtist or a name and returns true if artists contains an entry with the
same normalized name or matching spotifyUrl, then call this predicate from both
addFromSpotify and addManual before calling onUpdate/clearResults to prevent any
duplicate insertions into artists.
- Around line 72-111: The combobox is currently mouse-first; in
OnboardingArtistsStep add proper ARIA and keyboard handling by giving the Input
an id and role="combobox" plus aria-expanded and aria-controls pointing to the
dropdown id, track an activeIndex state and set aria-activedescendant to the
focused option id when showDropdown/showManualAdd is true, implement onKeyDown
on the Input to handle ArrowDown/ArrowUp to move activeIndex and Enter/Space to
call addFromSpotify(results[activeIndex]) or addManual, change the option
elements in the dropdown (rendered by ArtistSearchResult and the manual button)
to have role="option", unique ids, highlight based on activeIndex, and use
onClick (and also handle onKeyDown for Enter/Space) instead of onMouseDown so
keyboard selection and screen readers work correctly while keeping existing
addFromSpotify/addManual handlers.

In `@components/Onboarding/OnboardingModal.tsx`:
- Around line 48-53: The modal currently opens without an accessible name; add a
DialogTitle (or set aria-labelledby on DialogContent) that updates to reflect
the current onboarding step (including the welcome and complete states) so
screen readers announce the modal; update the component that renders
DialogContent (the DialogContent element) to include a DialogTitle element (or
an id referenced by aria-labelledby) that is derived from the step
state/variable that drives the content (e.g., the same state used for
welcome/complete) and ensure the title text changes with the step.

---

Nitpick comments:
In `@components/Onboarding/OnboardingTasksStep.tsx`:
- Around line 7-8: Change the loose string types to a compile-time linked
role-id union: replace RoleTaskMap's key type (currently Record<string, ...>)
with a union derived from the shared onboarding config (e.g. keyof typeof
onboardingRoleConfig or an exported OnboardingRoleId type) and update the
roleType variable to that same union (roleType: OnboardingRoleId | undefined).
Import or derive the union from components/Onboarding/onboardingRoleConfig (use
keyof typeof onboardingRoleConfig if no explicit type is exported) so any
rename/typo in the shared config fails to compile instead of falling back to
defaults; update all related usages (RoleTaskMap, the roleType declaration, and
the lookup that falls back to default tasks) to use the new union type.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ffaf8955-f120-49cd-a76a-cc38b39a44cc

📥 Commits

Reviewing files that changed from the base of the PR and between 1df4f3d and af097bc.

📒 Files selected for processing (6)
  • components/Onboarding/OnboardingArtistsStep.tsx
  • components/Onboarding/OnboardingContextStep.tsx
  • components/Onboarding/OnboardingModal.tsx
  • components/Onboarding/OnboardingPulseStep.tsx
  • components/Onboarding/OnboardingStepDots.tsx
  • components/Onboarding/OnboardingTasksStep.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • components/Onboarding/OnboardingStepDots.tsx
  • components/Onboarding/OnboardingPulseStep.tsx

Comment on lines +17 to +18
const PROGRESS_STEPS: OnboardingStep[] = ["role", "context", "artists", "connections", "pulse", "tasks"];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd "OnboardingStepDots.tsx" -t f -x sed -n '1,120p' {}
printf '\n---\n'
rg -n '\b(type|interface)\s+Step\b' --glob '*.ts' --glob '*.tsx' --glob '*.d.ts'

Repository: recoupable/chat

Length of output: 1801


🏁 Script executed:

#!/bin/bash
# Read the OnboardingModal.tsx file to see the actual code at lines 17-18 and 54-63
fd "OnboardingModal.tsx" -t f -x cat -n {} | head -70

printf '\n---\n'

# Find and read useOnboarding hook to understand OnboardingStep type
fd "useOnboarding" -t f -x sed -n '1,50p' {}

Repository: recoupable/chat

Length of output: 6758


Remove undefined Step type assertion at line 62 and unnecessary casts.

Line 62 uses step as Step, but Step type is not defined anywhere. The component expects OnboardingStep. Additionally, lines 45 and 59 unnecessarily cast step to OnboardingStep when it's already typed as such from the hook.

🛠️ Suggested fix
const PROGRESS_STEPS: OnboardingStep[] = ["role", "context", "artists", "connections", "pulse", "tasks"];

-            {PROGRESS_STEPS.indexOf(step as OnboardingStep) + 1} of {PROGRESS_STEPS.length}
+            {PROGRESS_STEPS.indexOf(step) + 1} of {PROGRESS_STEPS.length}
             </span>
             </div>
-            <OnboardingStepDots current={step as Step} />
+            <OnboardingStepDots current={step} />

-  const isProgressStep = PROGRESS_STEPS.includes(step as OnboardingStep);
+  const isProgressStep = PROGRESS_STEPS.includes(step);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 17 - 18, The code
uses an undefined Step type and redundant casts for the step variable; update
OnboardingModal to remove the nonexistent Step assertion (e.g., remove "as Step"
at line 62) and delete unnecessary "as OnboardingStep" casts (lines ~45 and ~59)
so that the hook-returned step uses the existing OnboardingStep type directly;
verify PROGRESS_STEPS remains typed as OnboardingStep[] and adjust any local
variables or function signatures (e.g., the step parameter/constant and any
usages in render/handlers) to rely on OnboardingStep without additional casts.

Comment on lines +28 to +32
useEffect(() => {
if (step === "tasks") {
persist(data);
}
}, [step]); // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't let completion outrun persistence.

persist(data) is fire-and-forget here, but components/Onboarding/useOnboardingPersist.ts:42-63 performs a plain fetch("/api/account/update"), and Line 41 hard-navigates shortly after completion. On a slow connection that request can be canceled before the onboarding flag lands, so the wizard can reopen on the next load. Gate the finish path on a settled persist, or send the write with keepalive/sendBeacon.

Also applies to: 34-42

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@components/Onboarding/OnboardingModal.tsx` around lines 28 - 32, The effect
calling persist(data) in OnboardingModal's useEffect is fire-and-forget and can
be canceled by navigation; update the finish flow to await the persist operation
(or use navigator.sendBeacon / fetch with keepalive) before proceeding to the
hard navigation so the onboarding flag is reliably written. Specifically, change
the logic around persist(...) in the effect and the code paths that trigger the
hard navigation so they either await the Promise returned by persist (from
components/Onboarding/useOnboardingPersist.ts's persist function) or switch the
POST to useBeacon/keepalive in useOnboardingPersist so the write survives
navigation; ensure any callers (the "tasks" step handler) are updated to depend
on the settled result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant